Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: extend timeout for doc build version fetch #32511

Closed
wants to merge 1 commit into from
Closed

tools: extend timeout for doc build version fetch #32511

wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Mar 26, 2020

5s is too short, one of our CI CentOS 7 machines fails on this, @MylesBorins noted that it regularly happens when he's doing releases. I don't know why this machine has problems (I'm updating it and restarting it atm) with a 5s timeout but it does.

Screenshot 2020-03-27 09 27 23

Confirmed on that machine that extending the timeout gets it through doc-only.

Is 30s too much? The timeout is there to handle the no-internet case.

@rvagg rvagg requested review from MylesBorins and richardlau March 26, 2020 22:57
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Mar 26, 2020
@rvagg
Copy link
Member Author

rvagg commented Mar 26, 2020

https://ci.nodejs.org/job/node-test-commit-linux/33761/nodes=centos7-64-gcc6/console failure example

addaleax
addaleax approved these changes Mar 26, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, although imo we really really should not be doing this at all, or at least cache the results for a minute or two so that make test/make doc doesn’t request the same files 30 times from GitHub over and over.

@rvagg
Copy link
Member Author

rvagg commented Mar 26, 2020

yes, I was going to open another issue about that, it's really tedious to watch this in action and when you imagine all those requests spread across our CI for each file then it's not really fair to GitHub (although I'm sure they can handle it!)

@rvagg rvagg mentioned this pull request Mar 26, 2020
Closed
MylesBorins
MylesBorins approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as a temporary solution to at least stop CI from failing.

Honestly seems like we could avoid this problem all together by keeping a list of supported versions in the source rather than hitting the nework for this

@addaleax
Copy link
Member

Honestly seems like we could avoid this problem all together by keeping a list of supported versions in the source rather than hitting the nework for this

@MylesBorins That’s what the tool falls back to when no network is available – but it intentionally tries to fetch from upstream first because the local copy in the source tree might be outdated (e.g. when on older branches)

@richardlau
Copy link
Member

As I have a day off work today I've had some time to finish off a refactoring of the previous versions logic and submitted #32518 which should help (also bumped the timeout in that to match what's been PR'ed here).

@rvagg
Copy link
Member Author

rvagg commented Apr 1, 2020

fixed in #32518 I think

@rvagg rvagg closed this Apr 1, 2020
@rvagg rvagg deleted the rvagg/extend-version-fetch-timeout branch April 1, 2020 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@codebytere codebytere codebytere approved these changes

@addaleax addaleax addaleax approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rvagg @addaleax @richardlau @MylesBorins @codebytere @nodejs-github-bot